-
Notifications
You must be signed in to change notification settings - Fork 676
Allow emails
table to contain multiple emails per user
#11642
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
f6ebfe0
to
77ebf49
Compare
77ebf49
to
edf516e
Compare
-- Set `is_primary` to true for existing emails | ||
UPDATE emails SET is_primary = true; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we currently have 72k rows in the emails
table. I'm a bit scared about running this as part of the migration, since the running migration will block the API server from restarting.
@eth3lbert any thoughts on this?
I guess we could extract the add column is_primary
above to a dedicated migration/PR, deploy that to run the migration, then manually run the update is_primary = true
without blocking the API server, and then merge/deploy the rest of this migration 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess we could extract the
add column is_primary
above to a dedicated migration/PR, deploy that to run the migration, then manually run theupdate is_primary = true
without blocking the API server, and then merge/deploy the rest of this migration 🤔
Yeah, I second this! I think this would be the most proper way to minimize potential issues caused by the introduced database migration.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I split this into a separate migration and will open a separate PR including just that.
Haven't forgotten about this! Thanks for your review – aiming to address your comments soon. |
This PR is split from #11629 – it contains just the database schema-related changes, with no changes to the API, frontend, or any user-visible behaviour, barring a change to the description of three properties of the
/api/v1/me
response in the OpenAPI schema.I previously had a note here about it being possible for an email address to exist on more than one user account, but this is the current behaviour in production so I have removed it.
Relates to #11597.